Optimize checkbox selection logic in backup dialog objects tree. #9110 #9501
Conversation
WalkthroughUpdates CSS styling in ProcessDetails component (overflow, minHeight properties), refactors PgTreeView selection logic and rendering with simplified state model and new DefaultNode component, removes PgTreeSelectionContext export, and strips ESLint directives from webpack configuration files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @web/pgadmin/misc/bgprocess/static/js/ProcessDetails.jsx:
- Line 34: The root StyledBox currently sets overflow: 'auto' which together
with the logs container (the logs section element that has overflow: 'auto' and
flexGrow: 1) can create two independent scroll contexts; remove or change the
root-level overflow (in the StyledBox definition) so it does not create its own
scroll (e.g. remove overflow or set it to 'visible'/'hidden') and ensure only
the logs container handles scrolling—verify that the logs container (the element
with flexGrow: 1 and overflow: 'auto') has the required height/flex parent so it
is the sole scrollable area.
In @web/pgadmin/static/js/PgTreeView/index.jsx:
- Around line 83-116: The toggleCheck handler currently collects only the
toggled node's checked descendants in selectedChNodes; when ancestor nodes
become fully checked inside the ancestor loop (in toggleCheck, lines around the
parent traversal), add those parent nodes to selectedChNodes when you set
newState[parent.id] = true so selectionChange receives them too; keep using the
existing symbols (toggleCheck, updateDescendants, selectedChNodes, parent
traversal, setCheckedState, selectionChange) and after the ancestor loop call
selectionChange with the augmented selectedChNodes. Optionally, inside
updateDescendants short-circuit when newState[n.id] === val to avoid needless
recursion and replace the hard-coded '__root__' check with a robust root
detection (e.g., parent === null or a configurable root id).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/static/js/PgTreeView/index.jsx (1)
192-198: Fix PropTypes to match component signature.The PropTypes definition doesn't match the actual component props. The component signature (line 157) expects
isCheckedandonToggle, but PropTypes listtreeandonNodeSelectionChangeinstead.🔎 Proposed fix for PropTypes
DefaultNode.propTypes = { node: PropTypes.object, style: PropTypes.any, - tree: PropTypes.object, + isChecked: PropTypes.oneOfType([PropTypes.bool, PropTypes.oneOf(['indeterminate'])]), + onToggle: PropTypes.func, hasCheckbox: PropTypes.bool, - onNodeSelectionChange: PropTypes.func };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/pgadmin/misc/bgprocess/static/js/ProcessDetails.jsxweb/pgadmin/static/js/PgTreeView/index.jsxweb/webpack.config.jsweb/webpack.shim.js
💤 Files with no reviewable changes (2)
- web/webpack.config.js
- web/webpack.shim.js
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/static/js/PgTreeView/index.jsx (1)
web/pgadmin/static/js/tree/tree.js (2)
node(19-19)Tree(78-596)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
- GitHub Check: run-python-tests-pg (windows-latest, 13)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (5)
web/pgadmin/static/js/PgTreeView/index.jsx (2)
80-80: LGTM! Simplified state management.The map-based
checkedStateapproach is cleaner and more performant than the previous context-based solution for tracking checkbox states.
118-148: LGTM! Cleaner rendering without context provider.The simplified Tree rendering correctly passes
isChecked,onToggle, andhasCheckboxprops to the Node component. Removing the context provider reduces complexity.web/pgadmin/misc/bgprocess/static/js/ProcessDetails.jsx (3)
42-43: LGTM!The
minHeightensures the command section maintains a minimum visible area, andoverflow: 'auto'appropriately handles long command text. These styling additions improve the component's layout consistency.
62-62: LGTM!The
minHeight: '120px'ensures the logs section maintains a reasonable minimum visible area, which works well with the existingflexGrow: 1property.
154-154: LGTM!Removing the redundant
displayandflexDirectionprops is appropriate since they're already defined in theStyledBoxstyled component (lines 30-31). This cleanup improves code maintainability.
adityatoshniwal
force-pushed
the
9110-object-perf
branch
from
f2c66a5 to
b4de725
Compare
2 participants
Summary by CodeRabbit
Bug Fixes
Style
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.